-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(buttonMoveIconsIconProp rule): transform to self closing tag #728
feat(buttonMoveIconsIconProp rule): transform to self closing tag #728
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! One non-blocking thought.
if (node.closingElement) { | ||
const closingSymbol = context | ||
.getSourceCode() | ||
.getLastToken(node.openingElement)!; | ||
|
||
fixes.push( | ||
fixer.replaceText(closingSymbol, " />"), | ||
fixer.remove(node.closingElement) | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think it would be worth helperizing this? Seems like something that would be useful in a lot of situations.
Though if we do helperize it we should probably add a check to only make an element self closing if it doesn't have any children.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the helper.
About checking that element has no children - this was problematic, because for example in this button rule, we remove the children in a fix and at the same time this fix for removing closing element is applied (so the node still has children at the time the condition is checked). We would have to run the self-closing fix additionaly in a cleanup rule.
So I made the helper customizable where it is not allowed to remove children by default, but it can be overwritten.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the above lgtm
aa5d7e3
to
49506de
Compare
We should wait with merging #735 first. There will be conflicts and the helper will need updates, because we cannot remove all children content in case of non |
- also fixes Icon imported with alias - supports Icon / react-icons being used in children attribute
49506de
to
cbb89f7
Compare
Closes #694
Also supports Icon / react-icons being used in children attribute.